fix(rpc): Refactor `getrawtransaction` & RPC error handling #9049

upbqdn · 2024-11-21T15:05:31Z

Motivation

Close #8945.

Specifications & References

Solution

Fix the bugs described in bug: getrawtransaction output conflicts with zcashd output #8744. Note that zcashd stores transactions in orphaned blocks and responds to queries requesting such transactions with height to -1. Since Zebra doesn't keep orphaned blocks below the rollback limit, it now returns the error "No such mempool or blockchain transaction". If we wanted to match zcashd, we'd need to start storing orphaned blocks.
Fix the error codes for getrawtransaction and some other RPCs.
Related cleanups:
- Refactor the error handling for RPCs.
- Add references to the original error codes.
- Simplify prop tests and run them with arbitrary Networks.
- Don't derive Serialize & Deserialize for txids

Tests

Update and add

snapshot tests,
unit tests, and
prop tests

to check the new error codes.

Follow-up Work

None

PR Author's Checklist

The PR name will make sense to users.
The PR provides a CHANGELOG summary.
The solution is tested.
The documentation is up to date.
The PR has a priority label.

PR Reviewer's Checklist

The PR Author's checklist is complete.
The PR resolves the issue.

We don't need such a test anymore because the deserialization is handled by Serde now.

Co-authored-by: Alfredo Garcia <[email protected]>

arya2

Looks good to me, it would be nice to apply this suggestion but it could also be removed in the next PR so I don't think it's a blocker.

arya2 · 2024-12-11T01:40:24Z

The lightwalletd tests seem to be required now, they should pass once #9052 has merged. There was a timeout here: https://github.com/ZcashFoundation/zebra/actions/runs/12265821808/job/34222718084?pr=9049#step:12:2016,

but the same tests passed with rust stable so it was likely spurious.

arya2 · 2024-12-11T15:06:51Z

@mergify update

mergify · 2024-12-11T15:06:57Z

update

✅ Branch has been successfully updated

conradoplg

I think there might be a bug, I'm looking into it

conradoplg · 2024-12-11T15:23:50Z

Yep, e.g. zcash-cli -conf=$PWD/zcash.conf getrawtransaction 851bf6fbf7a976327817c738c489d7fa657752445430922d94c983c0b9ed4609 1 is returning

error code: -1
error message:
Invalid params: invalid type: string "1e4842fd4962d395cd9905b7030fd5cb54c8c30148468c600416e4f14a264d2c", expected an array of length 32.

on main it returns

{
  "hex": "01000000010000000000000000000000000000000000000000000000000000000000000000ffffffff025100ffffffff0250c30000000000002321027a46eb513588b01b37ea24303f4b628afd12cc20df789fede0921e43cad3e875acd43000000000000017a9147d46a730d31f97b1930d3368a967c309bd4d136a8700000000",
  "height": 1,
  "confirmations": 2740688
}

upbqdn · 2024-12-11T18:52:37Z

Thanks for spotting the bug. I'll switch back to Strings and deserialize them manually instead of using transaction::Hash directly.

…fix-getrawtx

upbqdn · 2024-12-12T15:07:19Z

I think there might be a bug, I'm looking into it

Fixed.

Deriving `serde::Serialize` & `serde::Deserialize` for `transaction::Hash` was superfluous, and we didn't need it anywhere in the code.

upbqdn added 30 commits November 12, 2024 11:58

clean-up: simplify the def of MapServerError

6a27482

Use HexData instead of String for TXIDs

4827028

Remove a redundant test

1dcd49a

We don't need such a test anymore because the deserialization is handled by Serde now.

Adjust tests for using HexData

1cfe1cc

Make height and confirmations optional

9aa561d

Use legacy error codes

a973aab

fmt

7ccac98

Remove unneeded error codes

08234b7

Remove zebra-rpc/src/constants.rs

3247923

Rename MapServerError to MapError

f6da233

Rename OkOrServerError to OkOrError

4b88252

Allow specifying error codes when mapping errors

2c7aa71

Allow setting error codes when mapping options

91ed312

Use the right error code for getrawtransaction

943a815

fmt

c3d1e83

Add docs for the error conversion traits

415b919

Refactor the error handling for getblock

91361d8

Refactor error handling in sendrawtransaction

fb40ae0

Refactor the error handling for getblock

c23501a

Update the error handling for getrawtransaction

03e9b25

Refactor error handling for z_gettreestate

0d1d0c8

Refactor the error handling for address parsing

57325f0

Refactor the error handling for getrawtransaction

c29e410

Merge branch 'main' into fix-getrawtx

db48268

Update z_gettreestate snapshots

7777f3d

Cosmetics

a20b5f1

Merge branch 'main' into fix-getrawtx

2983b14

Refactor error handling in getblock

5696e0b

Refactor error handling in getblockheader

71186b5

Merge branch 'main' into fix-getrawtx

acac270

upbqdn and others added 4 commits December 10, 2024 13:36

Merge branch 'main' into fix-getrawtx

9f778e4

Use transaction::Hash instead of HexData

bdb8372

Simplify error handling

ed69359

Update zebra-rpc/src/server/error.rs

37d1a44

Co-authored-by: Alfredo Garcia <[email protected]>

upbqdn requested review from oxarbitrage and arya2 December 10, 2024 18:56

arya2 previously approved these changes Dec 10, 2024

View reviewed changes

Move a note on performance

2c342f7

upbqdn dismissed arya2’s stale review via 2c342f7 December 10, 2024 22:25

Fix a typo

74fe0f4

arya2 previously approved these changes Dec 10, 2024

View reviewed changes

Merge branch 'main' into fix-getrawtx

7eb3e95

conradoplg requested changes Dec 11, 2024

View reviewed changes

upbqdn added 5 commits December 12, 2024 13:17

Use String instead of transaction::Hash

6e1e011

Adjust and add proptests

ae6000f

Merge branch 'main' into fix-getrawtx

b54dd79

Reintroduce snapshots for invalid TXIDs

7db6ff6

Merge branch 'fix-getrawtx' of github.com:ZcashFoundation/zebra into …

bc32ddb

…fix-getrawtx

upbqdn dismissed arya2’s stale review via bc32ddb December 12, 2024 13:20

oxarbitrage mentioned this pull request Dec 12, 2024

add(rpc): getblock: return transaction details with verbosity=2 #9083

Merged

7 tasks

Don't derive Serialize & Deserialize for txids

c9088f8

Deriving `serde::Serialize` & `serde::Deserialize` for `transaction::Hash` was superfluous, and we didn't need it anywhere in the code.

conradoplg approved these changes Dec 13, 2024

View reviewed changes

mergify bot merged commit b0c4d19 into main Dec 13, 2024
171 checks passed

mergify bot deleted the fix-getrawtx branch December 13, 2024 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rpc): Refactor `getrawtransaction` & RPC error handling #9049

fix(rpc): Refactor `getrawtransaction` & RPC error handling #9049

upbqdn commented Nov 21, 2024 •

edited

Loading

arya2 left a comment

arya2 commented Dec 11, 2024 •

edited

Loading

arya2 commented Dec 11, 2024

mergify bot commented Dec 11, 2024

conradoplg left a comment

conradoplg commented Dec 11, 2024

upbqdn commented Dec 11, 2024

upbqdn commented Dec 12, 2024

fix(rpc): Refactor getrawtransaction & RPC error handling #9049

fix(rpc): Refactor getrawtransaction & RPC error handling #9049

Conversation

upbqdn commented Nov 21, 2024 • edited Loading

Motivation

Specifications & References

Solution

Tests

Follow-up Work

PR Author's Checklist

PR Reviewer's Checklist

arya2 left a comment

Choose a reason for hiding this comment

arya2 commented Dec 11, 2024 • edited Loading

arya2 commented Dec 11, 2024

mergify bot commented Dec 11, 2024

✅ Branch has been successfully updated

conradoplg left a comment

Choose a reason for hiding this comment

conradoplg commented Dec 11, 2024

upbqdn commented Dec 11, 2024

upbqdn commented Dec 12, 2024

fix(rpc): Refactor `getrawtransaction` & RPC error handling #9049

fix(rpc): Refactor `getrawtransaction` & RPC error handling #9049

upbqdn commented Nov 21, 2024 •

edited

Loading

arya2 commented Dec 11, 2024 •

edited

Loading